-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-2014)!: return the result of provided callback in withTransaction and withSession #3524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d68eb95 to
6fdf08b
Compare
src/collection.ts
Outdated
| Callback, | ||
| checkCollectionName, | ||
| DEFAULT_PK_FACTORY, | ||
| emitWarningOnce, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has nothing to do with this ticket specifically, but was failing the lint task on CI.
nbbeeken
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actual changes LGTM, but outstanding question about our approach here
| fn: WithTransactionCallback<T>, | ||
| options?: TransactionOptions | ||
| ): Promise<Document | undefined> { | ||
| withTransaction<T>(fn: WithTransactionCallback<T>, options?: TransactionOptions): Promise<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docs need a little update too:
* @remarks
* This function:
* - Will return the result of the withTransaction callback if every operation is successful
* - Will return `undefined` if the transaction is intentionally aborted with `await session.abortTransaction()`
* - Will throw if one of the operations throws or `throw` statement is used inside the `withTransaction` callback
Which makes me question, should we require that the callback return something truthy (via TS) or default to something truthy (maybe true) if the callback returns undefined? Otherwise, how can someone differentiate an intentionally aborted transaction and a committed one if they don't have a return value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning true sounds good if they haven't returned anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, I'm thinking we can just add value ?? true (this would also cover return null, but I think that's ok?) and a test for that case along with docs. I'll update the ticket AC just so we keep track
| return value; | ||
| } | ||
|
|
||
| return attemptTransactionCommit(session, startTime, fn, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it still be possible to get the legacy return value somehow (i.e. the Document returned by session.commitTransaction())?
I think it would be an acceptable breaking change in mongosh if that goes away (right now our own withTransaction helper returns it), but it might be nice to avoid this.
To avoid this, you could do some hackery to attach this secondary result to the returned Promise itself or return a tuple, e.g.
withTransaction<T>(fn: WithTransactionCallback<T>, options?: …): Promise<T> & { transactionResult: Promise<Document | undefined> }withTransaction<T>(fn: WithTransactionCallback<T>, options?: …): Promise<[callbackResult: T, transactionResult: Document|undefined]>
(that would also maybe solve the problem mentioned above about distinguishing the callback returning undefined from an aborted transaction?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only worry here is with this change withTransaction and withSession now diverge, and one of the goals of the ticket was to keep those two APIs the same for consistency. But I'm flexible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it could just be a separate function or item in options then that makes it return a tuple?
|
From slack: We're moving this effort down the road to the next major to get ahead of documentation updates that we would want to have ready before committing to this change. |
Description
Updates
withTransactionandwithSessionhelpers to return the return value of the user provided callback.What is changing?
session.withTransaction()now properly returns the return value of the callback.client.withSession()updates its signature to returnPromise<any>instead ofPromise<voidand returns the return value of the provided callback.Is there new documentation needed for these changes?
None
What is the motivation for this change?
NODE-2014
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript